Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only build the parser once #252

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Only build the parser once #252

merged 1 commit into from
Sep 29, 2023

Conversation

dburgener
Copy link
Owner

The initial implementation assumed that PolicyParser::new() was a relatively cheap function time-wise, and called it once per file. There's no need to rebuild it, and it turns out in lalrpop performance discussions that new() can actually be expensive (and possible performance improvements could move work into new() to speed up the steady state).

Regardless of the significance, there's no real cost to pulling new() out to call once per file (although it might complicate the API ever so slightly if we later expose parse_policy() as a public function.)

The benchmark run is below. The weird thing is that Stress Functions only has one file, so I would expect this to be a no-op for performance there. But that 14% has been pretty reproducible across multiple runs. I'm not really sure what's going on there.

Full system compile time: [6.1514 ms 6.2534 ms 6.3622 ms]
change: [-79.353% -77.290% -74.995%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high mild

Benchmarking Stress functions: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 10.2s, or reduce sample count to 40.
Stress functions time: [99.906 ms 102.88 ms 105.94 ms]
change: [-19.498% -13.892% -8.4060%] (p = 0.00 < 0.05)
Performance has improved.

The initial implementation assumed that PolicyParser::new() was a
relatively cheap function time-wise, and called it once per file.
There's no need to rebuild it, and it turns out in lalrpop performance
discussions that new() can actually be expensive (and possible
performance improvements could move work into new() to speed up the
steady state).

Regardless of the significance, there's no real cost to pulling new()
out to call once per file (although it might complicate the API ever so
slightly if we later expose parse_policy() as a public function.)

The benchmark run is below.  The weird thing is that Stress Functions
only has one file, so I would expect this to be a no-op for performance
there.  But that 14% has been pretty reproducible across multiple runs.
I'm not really sure what's going on there.

Full system compile     time:   [6.1514 ms 6.2534 ms 6.3622 ms]
                        change: [-79.353% -77.290% -74.995%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild

Benchmarking Stress functions: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 10.2s, or reduce sample count to 40.
Stress functions        time:   [99.906 ms 102.88 ms 105.94 ms]
                        change: [-19.498% -13.892% -8.4060%] (p = 0.00 < 0.05)
                        Performance has improved.
@dburgener dburgener merged commit c0c290e into main Sep 29, 2023
11 checks passed
@dburgener dburgener deleted the dburgener/parser-rebuild branch September 29, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants